-
Notifications
You must be signed in to change notification settings - Fork 9.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
core(a11y): add back html/tags/impact #5439
Conversation
} | ||
}, | ||
{ | ||
"node": { | ||
"type": "node", | ||
"selector": "body > a[href$=\"mailto:[email protected]\"]", | ||
"path": "3,HTML,1,BODY,29,A", | ||
"snippet": "<a href=\"mailto:[email protected]\" target=\"_blank\">" | ||
"snippet": "<a href=\"mailto:[email protected]\" target=\"_blank\"></a>", | ||
"axeFailureSummary": "Fix all of the following:\n Element is in tab order and does not have accessible text\n\nFix any of the following:\n Element does not have text that is visible to screen readers\n aria-label attribute does not exist or is empty\n aria-labelledby attribute does not exist, references elements that do not exist or references elements that are empty or not visible\n Element's default semantics were not overridden with role=\"presentation\"\n Element's default semantics were not overridden with role=\"none\"" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joelhsmith it seems like in several cases these can be static
Do you have a sense of which cases they're needed per-node? Is it just color-contrast?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking through axe-core, I found these:
aria-allowed-attr
aria-required-children
aria-required-parent
aria-valid-attr-value
aria-valid-attr
color-contrast
fieldset
group-labelledby
duplicate-id
non-empty-if-present
I would not be surprised in the future if even more of them became contextual in the future. Plus people can write their own checks which also might be contextual.
@@ -56,7 +59,7 @@ class AxeAudit extends Audit { | |||
extendedInfo: { | |||
value: rule, | |||
}, | |||
details: Audit.makeTableDetails(headings, items), | |||
details: {...Audit.makeTableDetails(headings, items), impact, tags}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should impact
and tags
be nested or something? I don't really have an opinion, this seems fine and nesting might not be any better, but it may be putting us on a path toward details
becoming the new grab bag :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I like it where it is.
If it were nested, I can't think of what it would be called. On Deque's site "impact" and "tags" meta is extrapolated to create the "Compliance Data & User Impact" column. "complianceUserImpact:" seems a little long anyway ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems ok. one question
@@ -42,7 +44,8 @@ class AxeAudit extends Audit { | |||
type: 'node', | |||
selector: Array.isArray(node.target) ? node.target.join(' ') : '', | |||
path: node.path, | |||
snippet: node.snippet, | |||
snippet: node.html || node.snippet, | |||
axeFailureSummary: node.failureSummary, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about just summary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussed in person, landed on explanation
to match with our audit-level naming 👍
Related Issues/PRs
closes #5402